KAFKA-17689: Remove unused TieredStorageTestHarness#22445
Merged
chia7712 merged 3 commits intoJun 8, 2026
Merged
Conversation
github-actions
Bot
added
triage
Russole
reviewed
Jun 3, 2026
Russole
left a comment
Russole
left a comment
There was a problem hiding this comment.
Thanks @joshua2519 for working on this. Mostly LGTM. Just left a few minor comments.
| # The Test Flow | ||
|
|
||
| Step 1: For every test, setup is done via TieredStorageTestHarness which extends IntegrationTestHarness and sets up a cluster with TS enabled on it. | ||
| Step 1: Each test is a standalone class. It declares a `clusterConfig()` method that returns a `ClusterConfig` with tiered storage enabled (via `TieredStorageTestUtils.createServerPropsForRemoteStorage`), and test methods annotated with `@ClusterTemplate("clusterConfig")` that receive a `ClusterInstance` provided by the test framework. |
There was a problem hiding this comment.
nit: The current wording says clusterConfig() returns a ClusterConfig, but the tiered storage tests generally return List<ClusterConfig> from their @ClusterTemplate methods. Could we make this slightly more precise, e.g. “returns one or more ClusterConfig instances, typically as List<ClusterConfig>”?
m1a2st
reviewed
Jun 6, 2026
m1a2st
left a comment
m1a2st
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @joshua2519 for this patch, two minor comments
| private static final Integer RLM_TASK_INTERVAL_MS = 500; | ||
| private static final Integer RLMM_INIT_RETRY_INTERVAL_MS = 300; | ||
|
|
||
| public static List<LocalTieredStorage> remoteStorageManagers(Collection<KafkaBroker> brokers) { |
Collaborator
There was a problem hiding this comment.
Since only TieredStorageTestContext uses this method, we can move it there.
| return storages; | ||
| } | ||
|
|
||
| public static List<BrokerLocalStorage> localStorages(Collection<KafkaBroker> brokers) { |
chia7712
reviewed
Jun 6, 2026
| return brokers.stream() | ||
| .map(b -> new BrokerLocalStorage(b.config().brokerId(), Set.copyOf(b.config().logDirs()), | ||
| STORAGE_WAIT_TIMEOUT_SEC)) | ||
| .collect(Collectors.toList()); |
…t, use Stream.toList(), and fix README wording
…-TieredStorageTestHarness # Conflicts: # storage/src/test/java/org/apache/kafka/tiered/storage/HarnessBackedClusterInstance.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nothing extends
TieredStorageTestHarnesssince the tiered-storageintegration tests moved to the
@ClusterTemplate+ClusterInstancepattern. This deletes it and its now-dead adapter
HarnessBackedClusterInstance, moves the two still-used static helpers(
remoteStorageManagers,localStorages) intoTieredStorageTestUtils, and refreshes the README. Test-only change.Reviewers: Ken Huang s7133700@gmail.com, Russole Chen
850905junior@gmail.com, Chia-Ping Tsai chia7712@gmail.com